-
Notifications
You must be signed in to change notification settings - Fork 39
[박준현] sprint3 #151
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[박준현] sprint3 #151
The head ref may contain hidden characters: "Basic-\uBC15\uC900\uD604-sprint3"
Conversation
addiescode-sj
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
수고하셨습니다!
주요 리뷰 포인트
- 미디어 쿼리 사용 관련 피드백
- 포맷팅
| } | ||
|
|
||
|
|
||
| @media screen and (max-width: 1199px) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
현재 CSS파일에서 max-width: 1199px -> max-width: 7687px 순서로 미디어쿼리가 작성되어있어 specificity 문제가 발생할수있습니다. 기본 스타일은 모바일에 맞추고, (작은 화면에서부터) 큰 화면으로 점차 확장해나가는 순서로 작성하시면 불필요한 스타일 재정의 및 코드 중복을 효과적으로 줄일 수 있습니다.
또, 미디어쿼리 조건 평가 시 max-width를 사용하게되면 max-width: 767px 이하의 화면에서는 두가지 미디어쿼리가 모두 적용됩니다. 이때 첫번째로 얘기한 specificity(특이성) 문제가 생길 수 있는데요! 두 미디어쿼리의 특이성이 동일하므로 나중에 선언된 스타일이 적용됩니다. 따라서 767px 이하 스크린에서 동일한 선택자의 속성을 재정의하지않으면 1199px이하에 적용되어있는 스타일이 동시에 적용됩니다. 관리에 그닥 좋지 않겠죠?
| } | ||
| .header_banner_imgbox, | ||
| .footer_banner_imgbox { | ||
| width: 700px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
반응형을 고려하면서 스크린 사이즈 변경에도 유연하게 대응하고싶다면 width를 쓰는것보다는 max-width, min-width 등으로 크기를 조절하면 좋습니다 :)
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
여기 공백은 엄청 많이 띄우셨네요!
포맷팅이 작업 퀄리티를 너무 쉽게 떨어지게 만드는 요소라서 다음부터는 포맷팅도 조금 신경써볼까요? :)
| color: #3692FF; | ||
| } | ||
|
|
||
| @media screen and (max-width: 767px) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 파일에서도 panda.css 에서 드린 코멘트 참고해서 리팩토링해보세요! :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
파일 이름이 panda.css라서 어떤 역할의 파일인지가 짐작이 안되네요!
index.html (메인페이지)의 css파일이라면 index.css 혹은 main.css와 같이 페이지 이름이 들어가게끔 네이밍 변경해볼까요?
| <picture class="logo_img"> | ||
| <source media="(max-width: 767px)" srcset="images/main-page/logo.svg"> | ||
| <img src="images/main-page/logo.png" alt="로고" class="logo_img"> | ||
| </picture> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
picture태그를 사용하셔도 괜찮지만 뷰포트에 따라 같은 이미지소스를 해상도만 다르게 최적화해서 보여주는 방식은 srcset, sizes 만으로도 충분합니다 :)
덧붙여, 첫 화면에 보이는 이미지가 아닌 스크롤을 쭉 내려야 보이는 이미지라면 레이지로딩을 적용할수도있고요 :)
아래 아티클 참고해보시고 리팩토링해볼까요?
요구사항
기본
심화
스크린샷
https://brilliant-liger-335e1c.netlify.app/
멘토에게